fix(datetime): scroll to newly selected date when value changes #27806

averyjohnston · 2023-07-14T19:12:07Z

Issue number: Resolves #26391

What is the current behavior?

When updating the value programmatically on an ion-datetime after it has already been created:

With grid style: The selected date visually updates, but the calendar does not scroll to the newly selected month.
With wheel style: The selected date does not visually update, i.e. the wheels do not move to show the newly selected date.

What is the new behavior?

Grid style datetimes now scroll to the selected date using the same animation as when clicking the next/prev month buttons.
- This animation mirrors the behavior in both MUI and native iOS. See the design doc for more information and screen recordings.
- The animation will not occur if the month/year did not change, or when the datetime is hidden.
Wheel style datetimes now visually update to the selected date. No animation occurs, also mirroring native.
The parseDate util has also had its type signatures updated to account for returning undefined when the date string is improperly formatted. This was missed when the util was refactored to support multiple date selection.

Does this introduce a breaking change?

Yes
No

Other information

Docs PR: docs(datetime): add best practices for setting value async ionic-docs#3053
While this can technically be considered a bug fix, we are merging it into a feature branch for safety; it's a fairly significant change to how datetime behaves, and may interfere with custom logic when updating a datetime's value async.
Jumping to the newly selected value is handled by replacing everything here with processValue(). This covers both wheel and grid datetimes.
activePartsClone as a whole was also removed. It was added in fix(datetime): update active calendar display when value changes #24244 to enable changing activeParts without triggering a rerender (and thus jumping to the new value) but since we now want to do that jump, the clone is no longer needed.
The animation code might be tricky to follow, so I recorded going through it: https://github.com/ionic-team/ionic-framework/assets/90629384/1afa5762-f493-441a-b662-f0429f2d86a7

stackblitz · 2023-07-14T19:12:10Z

Run & review this pull request in StackBlitz Codeflow.

…but seems to work)

…eRenderDate

core/src/components/datetime/utils/parse.ts

sean-perkins

I'm still in progress of manually testing. Dropping some early suggestions for changes.

core/src/components/datetime/datetime.tsx

liamdebeasi · 2023-07-19T20:22:00Z

core/src/components/datetime/datetime.tsx

+       * Animate smoothly to the forced month. This will also update
+       * workingParts and correct the surrounding months for us.
+       */
+      const targetMonthIsEarlier = isBefore(targetValue, workingParts);


Threading:

My understanding of the original issue is that asynchronously setting the datetime on load does not update the view. So if the datetime is going to be set either on load or shortly after load, will users even see this scrolling animation? For example, if a datetime in a modal has its value set as the modal opens, the scrolling would either happen on screen or as the modal transitions in.

The idea is mainly to mirror native behavior, which animates on both MUI and native iOS: https://github.com/ionic-team/ionic-framework-design-documents/blob/main/projects/ionic-framework/components/datetime/0003-datetime-async-value.md#references

@liamdebeasi Before I get too deep in bug fixes here, I wanted to check whether you still had any concerns with moving forward with the animation. I can understand wanting to avoid making datetime even more complex, and deciding to scrap the animation part would make fixing things up here easier, but there are a few reasons why I still feel we should go through with it:

The animation matches native behavior, as mentioned above.

The datetime won't always be hidden when the value is updated async. While that will definitely be our recommendation (see docs(datetime): add best practices for setting value async ionic-docs#3053), we should avoid glitchy-looking behavior when it isn't the dev's desired pattern. For example, an app could have a dropdown with appointments, and the datetime's value updates to reflect which one you've selected.

The animation is something we already decided on in the design doc, and I would rather commit to that than constantly be rethinking our decisions. This isn't a case where we missed something that dramatically changes the scope; we already recognized that the animation will be difficult, and decided to proceed anyway.

I agree we shouldn't omit the animation just because it is challenging. But I'd also like us to keep in mind that the primary objective is to update the UI, with the animation being a secondary goal. This behavior is similar to #23560 where it is closer to the spec, but it's not critical functionality.

I'm not advocating for removing this, but I do want to make sure we are being cautious. If you recall the old IntersectionObserver implementation in datetime, that consumed a lot of our time before we eventually abandoned that approach in favor of a scroll listener.

core/src/components/datetime/datetime.tsx

averyjohnston · 2023-08-04T20:08:58Z

core/src/components/datetime/test/set-value/datetime.e2e.ts

-
-      // Open month/year picker
-      await monthYearButton.click();
-      await page.waitForChanges();
-
-      // Select October 2021
-      // The year will automatically switch to 2021 when selecting 10
-      await monthColumn.locator('.picker-item[data-value="10"]').click();
-      await ionChange.next();
-
-      // Close month/year picker
-      await monthYearButton.click();


These parts aren't needed anymore because changing the value already jumps to the new month.

liamdebeasi

Great work so far! I found one edge case, though the rest of this is working well.

liamdebeasi · 2023-08-08T15:01:46Z

core/src/components/datetime/datetime.tsx

+    const didChangeMonth = month !== workingParts.month || year !== workingParts.year;
+    const bodyIsVisible = el.classList.contains('datetime-ready');
+    const { isGridStyle, showMonthAndYear } = this;
+    if (isGridStyle && didChangeMonth && bodyIsVisible && !showMonthAndYear) {


If you set datetime to a time it still scrolls to another month and then abruptly resets to the selected month.

Example:

<ion-datetime value="2022-02-22T16:30:00"></ion-datetime> <script> setTimeout(() => { datetime.value = '16:30'; }, 2000); </script>

I fixed the wonky scrolling in fbf2f9c. The datetime now errors out if you do this, but it looks like this case (setting the value to just a time when the presentation is something other than just time) was never supported; I'm able to trigger the same error on main after messing with it a bit. (Screencast below.) I'm thinking it would be good to add a warning for this improper usage in a separate ticket. Thoughts?

2023-08-08.11-12-18.mp4

Ticket created: https://ionic-cloud.atlassian.net/browse/FW-4996

core/src/components/datetime/datetime.tsx

liamdebeasi · 2023-08-09T17:59:58Z

core/src/components/datetime-button/datetime-button.tsx

@@ -206,6 +206,10 @@ export class DatetimeButton implements ComponentInterface {
     */
    const parsedDatetimes = parseDate(parsedValues.length > 0 ? parsedValues : [getToday()]);

+    if (!parsedDatetimes) {


Unsure if this is a bug, but if I open the month/year picker as the view datetime begins to animate, the animation gets cancelled and the view never updates to the new value.

I'm actually seeing it update; maybe I'm just not doing it quickly enough? (Screencast below.) Even if there is a specific timing that borks it, though, I'm not too worried as long as the highlighted date is correct once you do scroll to it. Seems like a small edge case that could be addressed separately.

2023-08-16.14-18-57.mp4

I think I had outdated code as I can no longer repro the issue I mentioned. However, another bug exists:

Switching to the month/year picker as the animation starts seems to break date selection altogether.

datetime-break.mov

Tested on Chrome 116. I added the following code to datetime/test/display/index.html to get the datetime to update async:

setTimeout(() => { datetime.value = '2023-10-23T16:30' }, 2000);

I'm able to trigger that on main as well, using the next/prev month buttons. The animation goes through the same methods, so it's the same bug. Thoughts on addressing it separately?

2023-08-18.14-17-38.mp4

Yeah that works for me 👍

Ticket created: https://ionic-cloud.atlassian.net/browse/FW-4997

liamdebeasi · 2023-08-09T18:01:47Z

Also thoughts on getting a dev build and getting the community to test?

averyjohnston · 2023-08-16T19:51:49Z

Also thoughts on getting a dev build and getting the community to test?

Dev build posted in the original issue 👍

liamdebeasi

Great work!

Let's do a couple things before merging:

Rebase this off feature-7.4 since we want to get this into a feature release.
Wait a couple more days to give devs time to provide feedback re: the dev build. If there's no response by Wednesday I think it's fine to merge. (That way we give them a full week to test)

averyjohnston · 2023-08-21T14:51:51Z

I'm waiting on @sean-perkins's review anyway 👀 Definitely think this is worth getting an extra pair of eyes on.

core/src/components/datetime/datetime.tsx

playaz87 · 2023-09-11T08:23:25Z

Any ETA on when this fix will be released? This bug is currently blocking us from releasing an app.

liamdebeasi · 2023-09-11T12:52:51Z

As noted in #26391 (comment), this fix will be available in an upcoming minor release of Ionic.

m1nka · 2023-12-13T09:54:44Z

Was this feature ever released? We are experiencing the same problem. Thank you!

m1nka · 2023-12-13T11:19:21Z

Ok, I found a workaround using these instructions. Using @ViewChild('mydatepicker') mydatepicker: IonDateTime; and the this.mydatepicker.reset(); works fine.

I think it would make a lot of sense to reset automatically in case popover/modal is closed / reopened.

liamdebeasi · 2023-12-13T14:24:48Z

Was this feature ever released? We are experiencing the same problem. Thank you!

This feature was released in Ionic 7.4.0: https://ionic.io/blog/announcing-ionic-7-4

amandaesmith3 added 2 commits July 14, 2023 12:58

always jump to selected month when value changes

ce4c2eb

remove remaining activePartsClone refs

39d6871

github-actions bot added the package: core @ionic/core package label Jul 14, 2023

amandaesmith3 added 12 commits July 17, 2023 14:57

add ability to animate smoothly to month of new value (needs cleanup …

5c13d59

…but seems to work)

remove DatetimeMonth interface and include day when forcing month

e6a9ed5

rename state variable to use date instead of month

5ae63ec

remove missed instance of manually getting forced day

27d76c7

nevermind this totally works, was jumping to another year lol

237f015

actually wait for next/prevMonth call to finish before unsetting forc…

e79a558

…eRenderDate

remove console logs

59e0bde

check if we actually scrolled to the forced month before returning it

db126be

pull grid style check into private getter

2877537

remove unused variables

21e3648

don't animate if in closed datetime-button

f90bcc4

switch lock promise to a local variable

79b8e92

averyjohnston commented Jul 18, 2023

View reviewed changes

core/src/components/datetime/utils/parse.ts Show resolved Hide resolved

amandaesmith3 added 2 commits July 18, 2023 14:13

more comments

69267e4

lint

ab9bf27

Base automatically changed from feature-7.2 to main July 19, 2023 18:24

add tests

cbfc8e9

averyjohnston changed the base branch from main to feature-7.3 July 19, 2023 19:01

Merge branch 'feature-7.3' into FW-3846

4cf6270

averyjohnston mentioned this pull request Jul 19, 2023

docs(datetime): add best practices for setting value async ionic-team/ionic-docs#3053

Merged

averyjohnston marked this pull request as ready for review July 19, 2023 19:28

averyjohnston requested review from liamdebeasi and sean-perkins as code owners July 19, 2023 19:28

sean-perkins reviewed Jul 19, 2023

View reviewed changes

core/src/components/datetime/datetime.tsx Outdated Show resolved Hide resolved

core/src/components/datetime/datetime.tsx Outdated Show resolved Hide resolved

liamdebeasi reviewed Jul 19, 2023

View reviewed changes

core/src/components/datetime/datetime.tsx Show resolved Hide resolved

core/src/components/datetime/datetime.tsx Outdated Show resolved Hide resolved

amandaesmith3 added 2 commits August 4, 2023 14:40

Merge remote-tracking branch 'origin/feature-7.3' into FW-3846

cb3f835

remove unneeded manual month/year selection from test

b1ccb75

averyjohnston commented Aug 4, 2023

View reviewed changes

averyjohnston requested a review from liamdebeasi August 4, 2023 20:24

liamdebeasi reviewed Aug 8, 2023

View reviewed changes

don't animate if target month and/or year are undefined

fbf2f9c

averyjohnston requested a review from liamdebeasi August 8, 2023 16:19

liamdebeasi reviewed Aug 8, 2023

View reviewed changes

core/src/components/datetime/datetime.tsx Outdated Show resolved Hide resolved

combine didChangeMonth and monthYearDefined conditions

b85c7df

averyjohnston requested a review from liamdebeasi August 8, 2023 18:44

liamdebeasi reviewed Aug 9, 2023

View reviewed changes

averyjohnston requested a review from liamdebeasi August 16, 2023 19:52

liamdebeasi approved these changes Aug 21, 2023

View reviewed changes

averyjohnston changed the base branch from feature-7.3 to feature-7.4 August 21, 2023 14:50

sean-perkins approved these changes Aug 21, 2023

View reviewed changes

core/src/components/datetime/datetime.tsx Outdated Show resolved Hide resolved

amandaesmith3 and others added 4 commits August 21, 2023 10:21

add isGridStyle to destructure from this

3d79ec9

Merge branch 'feature-7.4' into FW-3846

419b12a

lint

129b370

Merge branch 'feature-7.4' into FW-3846

d884163

averyjohnston merged commit 32244fb into feature-7.4 Aug 23, 2023

averyjohnston deleted the FW-3846 branch August 23, 2023 18:49

averyjohnston mentioned this pull request Aug 23, 2023

bug: Setting ion-datetime value programatically does not update the grid #26391

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(datetime): scroll to newly selected date when value changes #27806

fix(datetime): scroll to newly selected date when value changes #27806

averyjohnston commented Jul 14, 2023 •

edited

Loading

stackblitz bot commented Jul 14, 2023

sean-perkins left a comment

liamdebeasi Jul 19, 2023

averyjohnston Jul 19, 2023

averyjohnston Jul 20, 2023

liamdebeasi Jul 20, 2023 •

edited

Loading

averyjohnston Aug 4, 2023

liamdebeasi left a comment •

edited

Loading

liamdebeasi Aug 8, 2023

averyjohnston Aug 8, 2023

averyjohnston Aug 21, 2023

liamdebeasi Aug 9, 2023

averyjohnston Aug 16, 2023

liamdebeasi Aug 18, 2023 •

edited

Loading

averyjohnston Aug 18, 2023

liamdebeasi Aug 18, 2023

averyjohnston Aug 21, 2023

liamdebeasi commented Aug 9, 2023

averyjohnston commented Aug 16, 2023

liamdebeasi left a comment

averyjohnston commented Aug 21, 2023

playaz87 commented Sep 11, 2023

liamdebeasi commented Sep 11, 2023

m1nka commented Dec 13, 2023

m1nka commented Dec 13, 2023

liamdebeasi commented Dec 13, 2023

fix(datetime): scroll to newly selected date when value changes #27806

fix(datetime): scroll to newly selected date when value changes #27806

Conversation

averyjohnston commented Jul 14, 2023 • edited Loading

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

stackblitz bot commented Jul 14, 2023

sean-perkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamdebeasi Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamdebeasi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamdebeasi Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamdebeasi commented Aug 9, 2023

averyjohnston commented Aug 16, 2023

liamdebeasi left a comment

Choose a reason for hiding this comment

averyjohnston commented Aug 21, 2023

playaz87 commented Sep 11, 2023

liamdebeasi commented Sep 11, 2023

m1nka commented Dec 13, 2023

m1nka commented Dec 13, 2023

liamdebeasi commented Dec 13, 2023

averyjohnston commented Jul 14, 2023 •

edited

Loading

liamdebeasi Jul 20, 2023 •

edited

Loading

liamdebeasi left a comment •

edited

Loading

liamdebeasi Aug 18, 2023 •

edited

Loading